-
Notifications
You must be signed in to change notification settings - Fork 49
Create provisional and compat features around the vect type #328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create provisional and compat features around the vect type #328
Conversation
6e31f31 to
1d81bd5
Compare
|
Verification #14005157: ❌ fail |
1d81bd5 to
b2a4cfd
Compare
|
Verification #14009292: ❌ fail |
b2a4cfd to
c587b73
Compare
|
Verification #14009657: ❌ fail |
c587b73 to
015229f
Compare
|
Verification #14010442: ❌ fail |
|
Verification #14010884: ✅ pass |
The duplicated headers are unfortunate, and is there because one of them mentions "1.4" specifically. It's not a problem if we let the 1.2 versions rot.
72156e4 to
a4065c4
Compare
| log_level = 2; | ||
| } else { | ||
| } else if ($dev._compat_warning_statement) { | ||
| _warning "The write_only template only makes sense on registers." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get why this works. Why don't you get ESYNTAX on this when the compat feature is disabled? #if shouldn't stop the AST from being built. Or... does it??!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, it's because we suppress the warning while building dml-builtins.dmlast. After that the parser will not look at dml-builtins.dml again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good lord. So you're saying this code is only meaningful because we can leverage dmlast. Let me see if I get this right:
- The
.dmlastis prebuilt, and it is so with the compat feature enabled. Because of that, it's able to use the_warningstatement without provokingESYNTAX, no matter if the compat feature is enabled/disabled for the individual device. - But the value of
$dev._compat_warning_statementis resolved only when it's time to actually build the device. Because of that, if the individual device does not have that compat feature enabled, then compilation will end up leveragingerrorinstead of_warning.
Is that correct? That's...
I...
I don't like that. I don't like that we have code in dml-builtins/utility that only works because of dmlast caching; because we're able to parse and build the AST using a different set of flags compared to when building the device. That feels extremely bad.
Not to mention, people (at least I) look towards dml-builtins/utility for reference and inspiration -- this code snippet could leave people thinking that the pattern
#if ($dev._compat_warning_statement) {
_warning "...";
}
would work in general, when the fact is it only works due to extremely niche circumstances. Worse yet, they may extrapolate to expect that pattern would work for other syntactical features controlled by a compatibility feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You must at the very least add a comment about this. This should be treated as a HACK. A pretty egregious one at that -- you would never expect the correctness of a piece of model code to be dependent on whether or not it gets cached.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can add a comment.
Not to mention, people (at least I) look towards dml-builtins/utility for reference and inspiration
If you are in search of inspiration, then visiting to DML 1.2 code is strongly discouraged. Not good for your health.
(I would also have been concerned about the hackiness of this code if this wasn't dying code)
py/dml/provisional.py
Outdated
| } | ||
| ``` | ||
| * DML natively supports `vect` types in `foreach` statements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only true in DML 1.2!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test to prove you wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment to prove you proving me wrong, wrong!
a4065c4 to
a4864cf
Compare
|
PR Verification: ✅ success |
|
PR Verification: ❌ failure |
a4864cf to
acea79a
Compare
|
PR Verification: ✅ success |
acea79a to
70d4d0e
Compare
lwaern-intel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my existential crisis re. abusing .dmlast caching
|
PR Verification: ✅ success |
On popular demand. Also, rename ECVECT -> EOLDVECT and c_vect_without_provisional -> experimental_vect
70d4d0e to
8e97202
Compare
lwaern-intel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved modulo verification. Though I do feel inclined to bikeshed a bit.
| error unless the [`experimental_vect` compatibility | ||
| feature](deprecations-auto.html#experimental_vect) is enabled. | ||
| ''' | ||
| short = "Allow vect syntax based on the VECT macro" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined to bikeshed this short desc a bit, for multiple reasons:
- To make it more obvious as to what the syntax actually is and does (it's possibly to read this as the DML syntax also being
VECT) - To explicitly state the connection to the Simics API
- And to somewhat future-proof the formulation against the introduction of RAII vectors
My suggestion would be:
Enable 'vect' type syntax that compile to usages of the Simics 'VECT' macro
However, I don't insist on any on this, and this is not blocking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to leave disambiguation to the full docs. I guess most of the time when you look at the list it's one of the other features you are looking for, which means the main use case for the short desc is to quickly convince you that this is not the thing you are looking for. So better to be terse and simple. If you don't know what the VECT macro is about, then you don't use the feature and the short desc is not for you.
|
PR Verification: ✅ success |
vecttype